Skip to content

Refactor API allowedValues to derive values from enums - #13895

Open
dheeraj12347 wants to merge 8 commits into
apache:mainfrom
dheeraj12347:feature/allowedvalues-enum-refactor
Open

Refactor API allowedValues to derive values from enums#13895
dheeraj12347 wants to merge 8 commits into
apache:mainfrom
dheeraj12347:feature/allowedvalues-enum-refactor

Conversation

@dheeraj12347

Copy link
Copy Markdown
Contributor

Description

Refactor API parameter allowedValues discovery to support enum-backed values.

Previously, enum values were specified directly as hard-coded strings in API parameter annotations. This can result in duplicated sources of truth when the corresponding enum changes.

This change introduces allowedValueType to @Parameter. When an enum type is provided, API discovery derives the allowed values directly from the enum constants.

The existing explicit allowedValues mechanism is retained for parameters that do not have a corresponding enum.

The change also expands enum-backed allowed-values coverage across existing API parameters.

Changes

  • Add allowedValueType to @Parameter.
  • Derive allowed values from enum constants in ApiDiscoveryServiceImpl.
  • Retain explicit allowedValues support as a fallback.
  • Refactor existing hard-coded enum values to use their corresponding enums.
  • Add unit-test coverage for enum-based allowed-value discovery.

Validation

  • mvn -pl plugins/api/discovery -am -DskipTests compile
  • mvn -pl plugins/api/discovery -Dtest=ApiDiscoveryServiceImplTest -DfailIfNoTests=false test
  • git diff --check
  • Checkstyle: 0 violations

API Discovery tests: 8/8 passed.

@dheeraj12347

Copy link
Copy Markdown
Contributor Author

Hi @Pearl1594 and @soreana , I’ve opened PR #13895 with the updated allowedValues implementation.

I incorporated the work from #13783 into this branch and addressed the hard-coded enum concern by adding allowedValueType to @parameter and deriving the values from the enum constants during API discovery. I’ve also retained the existing explicit allowedValues path for parameters where there is no corresponding enum, and added test coverage for both paths.

I’ve verified the implementation with the API Discovery build, the API Discovery tests (8/8 passing), git diff --check, and checkstyle.

Regarding your earlier request to merge #13783 into #13543: since the #13783 work is now incorporated into #13895, should I still merge #13783 explicitly into #13543, or is #13895 intended to supersede that merge?

I also want to confirm whether you’d like me to do any further audit of the remaining explicit allowedValues entries where a corresponding enum may exist, before considering this complete.

@DaanHoogland

Copy link
Copy Markdown
Contributor

@blueorangutan package

@blueorangutan

Copy link
Copy Markdown

@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress.

@codecov

codecov Bot commented Aug 17, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 3.41%. Comparing base (a4a52c9) to head (37d4a44).
⚠️ Report is 199 commits behind head on main.

❗ There is a different number of reports uploaded between BASE (a4a52c9) and HEAD (37d4a44). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (a4a52c9) HEAD (37d4a44)
unittests 1 0
Additional details and impacted files
@@              Coverage Diff              @@
##               main   #13895       +/-   ##
=============================================
- Coverage     18.08%    3.41%   -14.68%     
=============================================
  Files          6037      487     -5550     
  Lines        542580    41867   -500713     
  Branches      66428     7912    -58516     
=============================================
- Hits          98149     1429    -96720     
+ Misses       433409    40238   -393171     
+ Partials      11022      200    -10822     
Flag Coverage Δ
uitests 3.41% <ø> (-0.11%) ⬇️
unittests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@blueorangutan

Copy link
Copy Markdown

Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 18890

@DaanHoogland

Copy link
Copy Markdown
Contributor

@dheeraj12347 :

18:57:12 [ERROR] /jenkins/workspace/acs-centos8-pkg-builder/dist/rpmbuild/BUILD/cloudstack-4.23.0.0-SNAPSHOT/api/src/main/java/org/apache/cloudstack/api/command/user/snapshot/ListSnapshotsCmd.java:[52,35] error: package DateUtil does not exist

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants